Skip to content

Fix ACL hook routing for internal proxy system resources#10372

Open
somiljain2006 wants to merge 2 commits into
apache:developfrom
somiljain2006:Proxy-acl-cluster-mode
Open

Fix ACL hook routing for internal proxy system resources#10372
somiljain2006 wants to merge 2 commits into
apache:developfrom
somiljain2006:Proxy-acl-cluster-mode

Conversation

@somiljain2006
Copy link
Copy Markdown

Which Issue(s) This PR Fixes

Brief Description

This PR fixes ACL authentication issues for internal proxy system-resource requests in cluster mode when enableAclRpcHookForClusterMode=true is enabled. Introduced SystemResourceAwareRpcHook and InternalContextHolder to ensure that only trusted internal proxy operations targeting RocketMQ system resources use proxy admin ACL credentials, while normal client requests continue using user credentials.

How Did You Test This Change?

Added comprehensive unit tests covering:

  • internal vs external request routing
  • system resource detection
  • fallback extFields handling
  • unregister client validation
  • response hook routing
  • ThreadLocal context propagation and cleanup

Manual verification was also performed successfully using NameServer + Broker + Proxy with ACL enabled.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.79012% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.90%. Comparing base (54708be) to head (f84f5f1).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
...ketmq/proxy/common/SystemResourceAwareRpcHook.java 63.15% 11 Missing and 10 partials ⚠️
.../rocketmq/proxy/service/ClusterServiceManager.java 0.00% 9 Missing ⚠️
...etmq/proxy/service/sysmessage/HeartbeatSyncer.java 50.00% 4 Missing ⚠️
...e/rocketmq/proxy/common/InternalContextHolder.java 85.71% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10372      +/-   ##
=============================================
- Coverage      48.96%   48.90%   -0.07%     
- Complexity     13473    13476       +3     
=============================================
  Files           1376     1378       +2     
  Lines         100546   100620      +74     
  Branches       12984    13000      +16     
=============================================
- Hits           49236    49211      -25     
- Misses         45310    45378      +68     
- Partials        6000     6031      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dingshuangxi888
Copy link
Copy Markdown
Contributor

dingshuangxi888 commented May 26, 2026

  1. Proxy到Broker的请求,应该走内置的账号,不应该走用户的账号,建议拥有全部权限的账号来访问。
  2. 用户账号不应该配置系统Topic的权限,不然每个用户账号都要配置系统资源的权限,这个体验会很差。

qianye1001

This comment was marked as low quality.

qianye1001

This comment was marked as low quality.

qianye1001

This comment was marked as low quality.

@qianye1001
Copy link
Copy Markdown
Contributor

这个 PR 用一个复杂的 dual-hook 路由机制,绕过了一个可能只是配置问题的 bug。 从架构上讲,这是一个反模式——它让 proxy 在某些路径上使用用户凭证跟 broker 通信,这本身就违背了 proxy 作为可信中间层的设计原则。

@somiljain2006
Copy link
Copy Markdown
Author

somiljain2006 commented May 26, 2026

Thanks for the review.
The intention was not to change the proxy trust model, but to ensure internal proxy-originated system traffic can use the built-in credentials instead of requiring end-user accounts to have permissions for internal system resources/topics.

I agree the current dual-hook routing introduces additional complexity at the RPC layer. If this direction does not align with the intended authentication architecture for the proxy, I’m open to close the PR or explore a simpler approach that better fits the existing design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] acl 2.0 system topic TBW102 and group CLIENT_INNER_PRODUCER need manual authorization when i use cluster proxy mode

4 participants